<fix>[storage]: return group tree in APIQueryVolumeSnapshotTree#4144
Open
zstack-robot-1 wants to merge 3 commits into
Open
<fix>[storage]: return group tree in APIQueryVolumeSnapshotTree#4144zstack-robot-1 wants to merge 3 commits into
zstack-robot-1 wants to merge 3 commits into
Conversation
Add groupTrees field to APIQueryVolumeSnapshotTreeReply, built from VolumeSnapshotGroupVO topology with vote-majority parent resolution. Extract build logic into VolumeSnapshotGroupTreeBuilder. Resolves: ZSV-9792 Change-Id: I71616f6f6f6c637a6167637863727575746f616c
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough该 PR 为卷快照查询接口增加“快照组树”支持：新增响应字段、库存模型、组树构建器、管理器集成，并重构删除后组清理逻辑。 Changes卷快照组树查询端到端实现
Sequence DiagramssequenceDiagram
participant Client as 查询客户端
participant Manager as VolumeSnapshotManagerImpl
participant Builder as VolumeSnapshotGroupTreeBuilder
participant DB as 数据库
Client->>Manager: APIQueryVolumeSnapshotTreeMsg (含 volumeUuid 条件)
Manager->>Manager: marshalReplyMessageBeforeSending
Manager->>Manager: marshalVolumeTrees
Manager->>Manager: marshalGroupTrees
Manager->>Manager: 提取 volumeUuid 条件并验证可见性
Manager->>DB: 验证 volumeUuid 是否在返回 inventories 列表中
alt 可见且定位到根 VM
Manager->>Builder: buildForVm(vmInstanceUuid, visibleVolumeUuids)
Builder->>DB: 查询 VolumeSnapshotGroup 与 VolumeSnapshotGroupRef
Builder->>DB: 加载未删除快照 VO
Builder->>Builder: 构建 parentMap 与 snapToGroup 映射
Builder->>Builder: 解析父组（投票选举）
Builder->>Builder: 组装森林并标记 current
Builder-->>Manager: 返回 List<VolumeSnapshotGroupTreeInventory>
Manager->>Manager: reply.setGroupTrees(...)
else 不匹配或失败
Manager->>Manager: 不设置 groupTrees
end
Manager-->>Client: APIQueryVolumeSnapshotTreeReply (inventories + groupTrees?)
预估代码审查工作量🎯 4 (Complex) | ⏱️ ~45 分钟 诗歌
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
MatheMatrix
force-pushed
the
sync/tao.gan/5.1.0-ZSV-4950@@2
branch
from
1137ca5 to
8f43cbe
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@storage/src/main/java/org/zstack/storage/snapshot/group/VolumeSnapshotGroupTreeBuilder.java`:
- Around line 267-281: In markCurrent(Map<String,
VolumeSnapshotGroupTreeInventory> groupNodeMap) ensure deterministic selection
and reset: iterate all VolumeSnapshotGroupTreeInventory entries to
setCurrent(false) first, then pick the newest by createDate and, when createDate
is equal or both null, use a stable tie-breaker such as comparing a stable
identifier (e.g., node.getUuid() or node.getName()) via String.compareTo to
deterministically choose one; finally call setCurrent(true) on the chosen newest
node. This touches the markCurrent method and VolumeSnapshotGroupTreeInventory
usage.
In
`@storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotManagerImpl.java`:
- Around line 1368-1390: marshalGroupTrees currently writes group trees from
groupTreeBuilder.buildForVm(vmInstanceUuid) into the reply without applying
session-level visibility pruning, which can leak snapshot details; update this
flow to pass the caller session (from APIQueryVolumeSnapshotTreeMsg or its
session/context) into the VolumeSnapshotGroupTreeBuilder and use a session-aware
builder method (e.g., buildForVm(session, vmInstanceUuid) or trimBySession(...)
inside VolumeSnapshotGroupTreeBuilder) so that snapshots the session cannot see
are reduced to only uuid (preserving the “no access -> only uuid” contract)
before calling reply.setGroupTrees; ensure the builder enforces field-level
trimming for unauthorized sessions.
In
`@storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.java`:
- Around line 2148-2182: The deletion path in VolumeSnapshotTreeBase marks
VolumeSnapshotGroupRefVO rows for snapshots in the local `snapshots` list as
snapshotDeleted=true but misses group refs for memory snapshots removed later in
cleanUpMemorySnapshots(), leaving remaining>0 and preventing group disband; fix
by also marking group refs for those memory snapshots as deleted—either include
memorySnapshotsOfDescendants' UUIDs when building `uuids` (or perform an
additional SQL.New(VolumeSnapshotGroupRefVO.class)...in(...)
.set(snapshotDeleted,true).update() for the memory snapshot UUIDs) before
computing `remaining` and deleting groups, so VolumeSnapshotGroupRefVO rows for
memory snapshots are set snapshotDeleted=true alongside regular snapshots.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 94ca3d14-5ce9-4116-b74b-d291930fa4d9
📒 Files selected for processing (9)
header/src/main/java/org/zstack/header/storage/snapshot/APIQueryVolumeSnapshotTreeReply.javaheader/src/main/java/org/zstack/header/storage/snapshot/APIQueryVolumeSnapshotTreeReplyDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/snapshot/group/VolumeSnapshotGroupTreeInventory.javaheader/src/main/java/org/zstack/header/storage/snapshot/group/VolumeSnapshotGroupTreeInventoryDoc_zh_cn.groovyheader/src/main/java/org/zstack/header/storage/snapshot/group/VolumeSnapshotGroupTreeRefInventory.javaheader/src/main/java/org/zstack/header/storage/snapshot/group/VolumeSnapshotGroupTreeRefInventoryDoc_zh_cn.groovystorage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotManagerImpl.javastorage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.javastorage/src/main/java/org/zstack/storage/snapshot/group/VolumeSnapshotGroupTreeBuilder.java
Comment on lines
+267
to
+281
| private void markCurrent(Map<String, VolumeSnapshotGroupTreeInventory> groupNodeMap) { | ||
| VolumeSnapshotGroupTreeInventory newest = null; | ||
| for (VolumeSnapshotGroupTreeInventory node : groupNodeMap.values()) { | ||
| if (newest == null) { | ||
| newest = node; | ||
| continue; | ||
| } | ||
| if (node.getCreateDate() != null && (newest.getCreateDate() == null | ||
| || node.getCreateDate().after(newest.getCreateDate()))) { | ||
| newest = node; | ||
| } | ||
| } | ||
| if (newest != null) { | ||
| newest.setCurrent(true); | ||
| } |
There was a problem hiding this comment.
为 current 选择增加稳定的并列判定
Line 274-276 只按 createDate 比较；当时间相同会受 HashMap 遍历顺序影响，current 结果不稳定，可能导致查询结果抖动。
🔧 建议修复
private void markCurrent(Map<String, VolumeSnapshotGroupTreeInventory> groupNodeMap) {
VolumeSnapshotGroupTreeInventory newest = null;
for (VolumeSnapshotGroupTreeInventory node : groupNodeMap.values()) {
if (newest == null) {
newest = node;
continue;
}
- if (node.getCreateDate() != null && (newest.getCreateDate() == null
- || node.getCreateDate().after(newest.getCreateDate()))) {
+ if (node.getCreateDate() != null && (newest.getCreateDate() == null
+ || node.getCreateDate().after(newest.getCreateDate())
+ || (node.getCreateDate().equals(newest.getCreateDate())
+ && node.getUuid().compareTo(newest.getUuid()) < 0))) {
newest = node;
}
}
if (newest != null) {
newest.setCurrent(true);
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@storage/src/main/java/org/zstack/storage/snapshot/group/VolumeSnapshotGroupTreeBuilder.java`
around lines 267 - 281, In markCurrent(Map<String,
VolumeSnapshotGroupTreeInventory> groupNodeMap) ensure deterministic selection
and reset: iterate all VolumeSnapshotGroupTreeInventory entries to
setCurrent(false) first, then pick the newest by createDate and, when createDate
is equal or both null, use a stable tie-breaker such as comparing a stable
identifier (e.g., node.getUuid() or node.getName()) via String.compareTo to
deterministically choose one; finally call setCurrent(true) on the chosen newest
node. This touches the markCurrent method and VolumeSnapshotGroupTreeInventory
usage.
Comment on lines
+2148
to
+2182
| List<String> uuids = snapshots.stream() | ||
| .map(VolumeSnapshotInventory::getUuid) | ||
| .collect(Collectors.toList()); | ||
| SQL.New(VolumeSnapshotGroupRefVO.class) | ||
| .in(VolumeSnapshotGroupRefVO_.volumeSnapshotUuid, uuids) | ||
| .set(VolumeSnapshotGroupRefVO_.snapshotDeleted, true) | ||
| .update(); | ||
|
|
||
| Set<String> touchedGroupUuids = snapshots.stream() | ||
| .map(VolumeSnapshotInventory::getGroupUuid) | ||
| .filter(Objects::nonNull) | ||
| .collect(Collectors.toSet()); | ||
| if (touchedGroupUuids.isEmpty()) { | ||
| return; | ||
| } | ||
|
|
||
| List<String> groupsToDelete = new ArrayList<>(); | ||
| for (String groupUuid : touchedGroupUuids) { | ||
| long remaining = Q.New(VolumeSnapshotGroupRefVO.class) | ||
| .eq(VolumeSnapshotGroupRefVO_.volumeSnapshotGroupUuid, groupUuid) | ||
| .eq(VolumeSnapshotGroupRefVO_.snapshotDeleted, false) | ||
| .count(); | ||
| if (remaining == 0) { | ||
| logger.debug(String.format("snapshot group[uuid:%s] all refs deleted, disbanding", groupUuid)); | ||
| groupsToDelete.add(groupUuid); | ||
| } | ||
| } | ||
|
|
||
| groupUuids.forEach(groupUuid -> vidm.deleteArchiveVmInstanceResourceMetadataGroup(groupUuid)); | ||
| cleanVmHostBackupFilesForGroup(groupUuids); | ||
| dbf.removeByPrimaryKeys(groupUuids, VolumeSnapshotGroupVO.class); | ||
| if (groupsToDelete.isEmpty()) { | ||
| return; | ||
| } | ||
|
|
||
| groupsToDelete.forEach(vidm::deleteArchiveVmInstanceResourceMetadataGroup); | ||
| cleanVmHostBackupFilesForGroup(groupsToDelete); | ||
| dbf.removeByPrimaryKeys(groupsToDelete, VolumeSnapshotGroupVO.class); |
There was a problem hiding this comment.
链路删除漏掉了内存快照的分组引用清理。
这里仅把 snapshots 里的 VolumeSnapshotGroupRefVO 标记为 snapshotDeleted=true，但同一条删除链路后面还会在 cleanUpMemorySnapshots() 里硬删 memorySnapshotsOfDescendants。这些 memory snapshot 的 group ref 没有一起置为 deleted，remaining 会一直大于 0，导致包含内存快照的快照组不会解散，VolumeSnapshotGroupVO 和归档元数据都会残留。
💡 修复方向
- if (!cleanup()) {
+ List<VolumeSnapshotInventory> snapshotsToUngroup = new ArrayList<>(currentLeaf.getDescendants());
+ snapshotsToUngroup.addAll(memorySnapshotsOfDescendants);
+ if (!cleanup(snapshotsToUngroup)) {
...- private boolean cleanup() {
+ private boolean cleanup(List<VolumeSnapshotInventory> snapshotsToUngroup) {
List<VolumeSnapshotInventory> snapshots = currentLeaf.getDescendants();
...
if (cleanup) {
- ungroupAfterDeleted(snapshots);
+ ungroupAfterDeleted(snapshotsToUngroup);
}
return cleanup;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@storage/src/main/java/org/zstack/storage/snapshot/VolumeSnapshotTreeBase.java`
around lines 2148 - 2182, The deletion path in VolumeSnapshotTreeBase marks
VolumeSnapshotGroupRefVO rows for snapshots in the local `snapshots` list as
snapshotDeleted=true but misses group refs for memory snapshots removed later in
cleanUpMemorySnapshots(), leaving remaining>0 and preventing group disband; fix
by also marking group refs for those memory snapshots as deleted—either include
memorySnapshotsOfDescendants' UUIDs when building `uuids` (or perform an
additional SQL.New(VolumeSnapshotGroupRefVO.class)...in(...)
.set(snapshotDeleted,true).update() for the memory snapshot UUIDs) before
computing `remaining` and deleting groups, so VolumeSnapshotGroupRefVO rows for
memory snapshots are set snapshotDeleted=true alongside regular snapshots.
Two P1 fixes from review: 1. parent map now includes deleted snapshots so chain traversal can cross the deleted point and reach ancestor group. 2. when query is scoped to a single volume, filter refs and groupVOs by the visible volume set in the reply to avoid leaking snapshots of sibling volumes the caller cannot see. Resolves: ZSV-9792 Change-Id: I582aa250977fcaf9f5a2f0368d5891518949e32b
Initial fix sliced refs by visibleVolumeUuids, which broke the incomplete computation (a single-deleted group looked fully-deleted when only the matching volume's ref survived the filter). Drop the ref-level slice: cross-account leak is already prevented by the inventories.anyMatch(volumeUuid) check above, and refs of a single VM share the VM owner anyway, so returning full per-VM group structure is the intended semantic. Resolves: ZSV-9792 Change-Id: I5d58d358c1ac8146ef505e120d491f4e347fb108
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add groupTrees field to APIQueryVolumeSnapshotTreeReply, built from
VolumeSnapshotGroupVO topology with vote-majority parent resolution.
Extract build logic into VolumeSnapshotGroupTreeBuilder.
Resolves: ZSV-9792
Change-Id: I71616f6f6f6c637a6167637863727575746f616c
sync from gitlab !10048